Skip to content

Iterative destruction of JSON containers (objects and arrays) - #1431

Closed
mistersandman wants to merge 3 commits into
nlohmann:developfrom
mistersandman:iterative_container_destruction
Closed

mistersandman wants to merge 3 commits into
nlohmann:developfrom
mistersandman:iterative_container_destruction

Conversation

@mistersandman

Copy link
Copy Markdown

Deeply nested JSON hierarchies may lead to stack overflows during destruction, since JSON containers call the destructor of their children recursively (see issue #832).

This change provides an iterative destruction of JSON containers, avoiding a deep call stack by maintaining an explicit stack on the heap.

…) instead of implicit recursive destruction in order to avoid stack overflows for deeply nested hierarchies
@mistersandman
mistersandman force-pushed the iterative_container_destruction branch from ed953be to d5ac351 Compare January 14, 2019 20:17
@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Jan 14, 2019
@nlohmann

Copy link
Copy Markdown
Owner

Thanks for the PR!

@coveralls

coveralls commented Jan 15, 2019 •

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.02%) to 99.976% when pulling df42d48 on mistersandman:iterative_container_destruction into e326df2 on nlohmann:develop.

@jaredgrubb

Copy link
Copy Markdown
Contributor

I applaud your effort here and the PR looks correct. However I found it very difficult to audit and read -- which makes me worry that it will be difficult to maintain.

The hardest thing here is that there are many levels of indentation, and worse, two nested while loops.

I think splitting out the loops into separate functions (and avoiding having to split the top-level switch-case) makes it much easier to audit and understand what it's doing.

Eg, a rough sketch:

               case value_t::object:
               {
                    if (!this->object->empty())
                    {
                         destroy_subelements_depth_first(this);
                    }

                    AllocatorType<object_t> alloc;
                    std::allocator_traits<decltype(alloc)>::destroy(alloc, object);
                    std::allocator_traits<decltype(alloc)>::deallocate(alloc, object, 1);
                    break;
                }
                //// Ditto for array ....

And then:

void destroy_subelements_depth_first(json_value *value)
{
    std::vector<std::pair<json_value*, value_t>> stack;
    stack.emplace_back(this, t);
    while (!stack.empty())
    {
        json_value* value;
        value_t type;
        std::tie(value, type) = stack.back();
        bool all_done;
        if (type == value_t::object)
        {
             all_done = destroy_object_subelements(stack, value);
        }
        else
        {
             all_done = destroy_array_subelements(stack, value);
        }
        if (all_done)
        {
             stack.pop_back();
        }
    }
}

@mistersandman

Copy link
Copy Markdown
Author

@jaredgrubb Thank you very much for your feedback and sorry for the late reply! I changed the code pretty much exactly according to your suggestion and added a few comments. It is indeed much better readable now 👍

@mistersandman
mistersandman force-pushed the iterative_container_destruction branch from b4f70c9 to df42d48 Compare February 21, 2019 21:46
@stale

stale Bot commented Mar 24, 2019

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale Bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Mar 24, 2019
@stale stale Bot closed this Mar 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state: please discuss please discuss the issue or vote for your favorite option state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants